Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow per-backend user pipeline settings #677

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

dehnert
Copy link
Contributor

@dehnert dehnert commented Mar 28, 2022

According to https://python-social-auth.readthedocs.io/en/latest/configuration/settings.html, "All settings can be defined per-backend by adding the backend name to the setting name, like SOCIAL_AUTH_TWITTER_LOGIN_URL". This changes user.py to actually use per-backend settings for options like USERNAME_IS_FULL_EMAIL.

Note that get_username is always called with a backend, but user_details isn't, so a different version of `setting' is needed for the two.

Types of changes

Please check the type of change your PR introduces:

  • Release (new release request)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no api changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • [X?] Lint and unit tests pass locally with my changes -- running tox reports an error in test_dummy both before and after my change
  • I have added tests that prove my fix is effective or that my feature works

Other information

Any other information that is important to this PR such as screenshots of how
the component looks before and after the change.

@dehnert
Copy link
Contributor Author

dehnert commented Mar 29, 2022

I'm confused that CI appears to have still not run.

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #677 (f493c68) into master (f27461d) will not change coverage.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##           master     #677   +/-   ##
=======================================
  Coverage   77.01%   77.01%           
=======================================
  Files         319      319           
  Lines        9677     9677           
  Branches     1036     1036           
=======================================
  Hits         7453     7453           
  Misses       2072     2072           
  Partials      152      152           
Flag Coverage Δ
unittests 77.01% <90.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
social_core/pipeline/user.py 82.60% <90.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f27461d...f493c68. Read the comment docs.

@nijel
Copy link
Member

nijel commented Apr 30, 2022

Can you please prepare a matching pull request for the documentation? https://github.com/python-social-auth/social-docs/

@dehnert
Copy link
Contributor Author

dehnert commented May 7, 2022

I think this change just makes the code match the already-existing documentation -- see https://github.com/python-social-auth/social-docs/blame/master/docs/configuration/settings.rst#L18, "All settings can be defined per-backend". Can you clarify what you're looking for in terms of doc changes?

@dehnert
Copy link
Contributor Author

dehnert commented Jul 8, 2024

I'm still running with this patch on one of my sites -- it looks like this PR has a conflict now, but (glancing at the conflict) it doesn't look hard to resolve, or like the conflict is from this issue having been fixed. I can go back and update this PR, but @nijel , can you elaborate on what you were looking for in terms of added docs, so I can resolve that too?

@nijel
Copy link
Member

nijel commented Jul 8, 2024

I most likely missed your previous reply, it indeed seems that this PR makes it match the documentation.

According to
https://python-social-auth.readthedocs.io/en/latest/configuration/settings.html,
"All settings can be defined per-backend by adding the backend name to
the setting name, like SOCIAL_AUTH_TWITTER_LOGIN_URL". This changes
user.py to actually use per-backend settings for options like
USERNAME_IS_FULL_EMAIL.

Note that get_username is always called with a backend, but user_details
isn't, so a different version of `setting' is needed for the two.
@dehnert
Copy link
Contributor Author

dehnert commented Jul 13, 2024

I've rebased, if you want to take a look and maybe merge it.

@nijel nijel merged commit ec8d84f into python-social-auth:master Jul 15, 2024
7 checks passed
@nijel
Copy link
Member

nijel commented Jul 15, 2024

Merged, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants